feat(desktop): make pull request reviews actionable#2510
Conversation
Unify comments and review decisions into a navigable timeline, keep lifecycle actions available to repository owners and their managed agents, and preserve source-channel context. Polish reviewer controls, project timestamps, and code-linked review interactions for a clearer end-to-end review flow.
Show the latest three review events by default, retain per-PR expansion choices across full collapse, and make the timeline entry point easier to scan.
Expect the review timeline's new default-expanded state instead of collapsing it before checking the inline comment.
|
🤖 ## Screenshots Add reviewerThe reviewer picker now opens as a centered, searchable dialog.
Review requestedReviewer activity appears in the chronological review timeline.
Changes requestedA change request carries the reviewer's explanation and warning treatment.
ApprovedApproval summaries live in the same review activity timeline.
Draft lifecycleOwners and managed agents can move the pull request through its lifecycle controls.
Inline reviewInline requests include a clickable code reference that opens the relevant diff line.
|
wpfleger96
left a comment
wpfleger96
left a comment
There was a problem hiding this comment.
Leaving consolidated review findings from two independent passes at head b6d9ac880. The one item I'd fix before merge is the review-decision timestamp ordering (inline on hooks.ts) — the rest are minor/nits, inline below. What held up well: the lifecycle status command reuses the existing merge-path trust gates (project_owner_identity + spawn_key_refusal, rejects the merged alias, clamps created_at), canReviewProjectPullRequest is one gate shared across all three call sites, and the E2E coverage asserts signed-event payloads rather than just UI state.
| ...(decision | ||
| ? [ | ||
| ["t", PR_CHANGES_REQUESTED_LABEL], | ||
| ...(!normalizedAnchor ? [["c", pullRequest.commit as string]] : []), |
There was a problem hiding this comment.
The approve path (PullRequestReviewCard.runReviewDecision) allocates its timestamp through nextProjectPullRequestReviewCreatedAt() so it always advances past the latest decision, but this request-changes path signs with plain now. If the same reviewer lands two opposite decisions in the same second, both events tie on created_at and reviewDecisionsForPullRequest falls back to event-id lexicographic order (projectPullRequests.mjs:267-271) — so the later decision can lose. Worst case the UI toasts "Changes requested" while the PR effectively stays approved. Rare for humans given dialog latency, but plausible for agents driving the UI, and the newly added approval -> request-changes direction has no E2E covering it (the old "rapid opposite decisions" test was removed in this PR).
I think the fix is to thread nextProjectPullRequestReviewCreatedAt(pullRequest, now) in as createdAt here when decision is set, so all review decisions (approve + top-level/inline change requests) share one PR-scoped monotonic path — plus a regression test for the same-second approve -> request-changes case.
| "channel_id must not be empty".into(), | ||
| )); | ||
| } | ||
| tags.push(tag(&["h", channel_id])?); |
There was a problem hiding this comment.
This only rejects an empty channel_id and then emits the original untrimmed string, but --channel is documented as a channel UUID and the desktop navigates directly to it. Worth parsing it as a UUID (consistent with the rest of the SDK) or at least trimming/validating shape before adding the h tag — right now the CLI passes anything through raw.
| <span>in</span> | ||
| <button | ||
| className="truncate font-medium text-foreground underline-offset-2 hover:underline" | ||
| onClick={() => void goChannel(sourceChannelId)} |
There was a problem hiding this comment.
Heads up that this link reads as verified provenance but the h tag is author-claimed and unauthenticated: the relay classifies kind 1617 as global-only, so no membership check ever runs against the claimed channel — any PR author can stamp any channel UUID here, including a private channel they're not in. Name resolution comes from the viewer's own channel list so nothing private leaks, and clicking just navigates, but I'd either note this in a comment or render it visibly as author-claimed rather than as a trusted "in #channel" link.
| { label: "second", seconds: 1 }, | ||
| ]; | ||
|
|
||
| if (elapsedSeconds >= 7 * 24 * 60 * 60) { |
There was a problem hiding this comment.
nit: this new >=7-day absolute-date branch has no unit test — projectsViewHelpers has no test file at all, and relativeTime is now the single time formatter for issues, PRs, commits, and inline comments. A small .test.mjs with the year-boundary case would be cheap insurance.
| <Dialog | ||
| onOpenChange={(open) => { | ||
| setApproveDialogOpen(open); | ||
| if (!open && !isApproving) setApprovalSummary(""); |
There was a problem hiding this comment.
nit: Escape still closes the dialog while the approval mutation is in flight — onOpenChange only guards the summary reset, not the close itself. The approval still lands and toasts, but the dialog vanishing mid-flight is a bit misleading. Consider blocking close while isApproving.
| data-testid={anchor ? "project-diff-line" : undefined} | ||
| data-testid={ | ||
| isFocused | ||
| ? "project-diff-focused-line" |
There was a problem hiding this comment.
nit: the project-diff-focused-line highlight persists indefinitely after inline-comment navigation — there's no clear-on-interaction. Cosmetic, fine to leave, just flagging.
| ["e", input.pullRequestId, "", "root"], | ||
| ["a", input.repoAddress], | ||
| ["p", input.targetOwner], | ||
| ["p", input.pullRequestAuthor], |
There was a problem hiding this comment.
nit: this mock always pushes both p tags, but the real Rust command dedupes when author == owner. Mock-fidelity only, no behavioral impact on current specs.
|
|
||
| Run `buzz --help` or `buzz <group> --help` for full usage. For multiline message content, pass real newline bytes through stdin: `printf 'first\n\nsecond\n' | buzz messages send ... --content -`. Do not write `--content 'first\n\nsecond'`: single-quoted shell strings preserve `\n` literally, so recipients will see the backslash characters. `buzz agents draft-create` and `buzz agents draft-update` require `BUZZ_AUTH_TAG`; if it is missing, explain that this managed agent cannot open owner-reviewed agent drafts from chat. | ||
|
|
||
| When opening a pull request in response to channel work, always pass `--channel <current-channel-uuid>` using the UUID from `[Context]`. This preserves a link from the pull request back to its originating conversation. |
There was a problem hiding this comment.
Observation, no action needed: agents following this guidance will embed channel UUIDs in globally-readable PR events. UUIDs are opaque and grant no access, but it does disclose private-channel identifiers relay-wide — I'm assuming that's the intended tradeoff for source-channel linking, just want it to be a conscious one.
2 participants
Summary
Pull request reviews now keep the decision and its explanation together: reviewers can comment or request changes from the same composer, approve with an optional summary, and jump from inline activity directly to the referenced diff line. Review activity is presented as a chronological timeline that starts with the latest three entries, remembers expansion choices, and keeps available lifecycle actions visible when fully collapsed.
Repository owners and their managed agents can also move pull requests between draft, open, and closed states, while source-channel metadata links the review back to the conversation that produced it. Reviewer selection, author identity, timestamps, profile previews, and project navigation have been polished to make the full review flow easier to scan and operate.
Validation
pnpm --dir desktop typecheckTest plan